Allow users to choose the bbox clamping mode #9128
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/9128
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
torchvision/transforms/v2/_meta.py
Outdated
| def transform(self, inpt: tv_tensors.BoundingBoxes, params: dict[str, Any]) -> tv_tensors.BoundingBoxes: | ||
| out = inpt.clone() | ||
| out.clamping_mode = self.clamping_mode | ||
| return out |
There was a problem hiding this comment.
@AntoineSimoulin @scotts in our chat I raised a potential problem with the clamping_mode leaking out of this transform. I think I made our conversation a lot more complicated that it needed to be. We can avoid the "leak" by just calling clone(), as we should, which returns a copy.
No need to make Compose() work like a context manager. I added corresponding tests.
| @@ -46,6 +46,14 @@ def is_rotated_bounding_format(format: BoundingBoxFormat) -> bool: | |||
| ) | |||
|
|
|||
|
|
|||
| # TODOBB consider making this a Literal instead. Tried briefly and got | |||
| # torchscript errors, leaving to str for now. | |||
| # CLAMPING_MODE_TYPE = Literal["hard", "soft", "none"] | |||
There was a problem hiding this comment.
Regarding the acceptable values, I'm not a fan of "none", it might be confused with None. Maybe "no-clamping" is better? Regardless, if that's OK with you I think we can leave this (nonetheless important!) bikeshedding for later PR.
There was a problem hiding this comment.
Agreed that None and "none" are bound (ha!) to get confused and that we should definitely choose something else.
There was a problem hiding this comment.
Should we consider having possible values be either "hard", "soft" and None (no value provided) instead of having the "none" value as a string?
There was a problem hiding this comment.
I think that would be a good option, we'll have to make sure this doesn't conflict with the None default parameter of ClampBoundingBoxes and its kernel. Let's consider this soon in a follow-up
As long as the box itself must have a value (and I think that's the case), then I think that makes sense. |
|
Hey @NicolasHug! You merged this PR, but no labels were added. |
Closes #8254
This PR:
clamping_modeparameter to to theBoundingBoxes()constructor which is stored as a metadata attribute. The default is "hard" for now because this is what happens inmain. We will change it to "soft" as soon as Adjust clamping for rotated bboxes #9112 is merged.clamping_modeparameter to all necessary kernels.clamping_modeparameter to theClampBoundingBoxes()method. The default here isNone, which defaults to the bbox'sclamping_modeattribute. @AntoineSimoulin @scotts that's a different default from what we had originally discussed, but I think it makes sense?SetClampingMode()transform which just sets theclamping_modeattribute of a bboc and can safely be used within aCompose()pipeline.I left a bunch of non-critical
TODOBBtodos in the code, which I'll address later, but before the release. The type checker is failing, I'll address before merging, please review regardless :)cc @vfdev-5